separate doc-level monitor query indices for externally defined monitors #1664

sbcd90 · 2024-09-25T03:17:39Z

Description

separate doc-level monitor query indices for externally defined monitors
Adds boolean flag in monitor config to deleteQueryIndexInEveryRun

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Subhobrata Dey <[email protected]>

eirsep · 2024-09-25T06:16:40Z

alerting/src/main/kotlin/org/opensearch/alerting/DocumentLevelMonitorRunner.kt

@@ -374,6 +374,13 @@ class DocumentLevelMonitorRunner : MonitorRunner() {
                // Clean up any queries created by the dry run monitor
                monitorCtx.docLevelMonitorQueries!!.deleteDocLevelQueriesOnDryRun(monitorMetadata)
            }
+
+            if (monitor.dataSources.queryIndex.contains("optimized")) {


plz add a flag in monitor creation called deleteQueryIndexBeforeEveryRun (or something briefer)

added as part of pr opensearch-project/common-utils#734

eirsep · 2024-09-25T06:17:09Z

alerting/src/main/kotlin/org/opensearch/alerting/DocumentLevelMonitorRunner.kt

+            if (monitor.dataSources.queryIndex.contains("optimized")) {
+                val ack = monitorCtx.docLevelMonitorQueries!!.deleteDocLevelQueryIndex(monitor.dataSources)
+                if (!ack) {
+                    logger.error("Deletion of concrete queryIndex:${monitor.dataSources.queryIndex} is not ack'd!")


plz log monitor id

eirsep · 2024-09-25T06:20:38Z

alerting/src/main/kotlin/org/opensearch/alerting/DocumentLevelMonitorRunner.kt

@@ -385,6 +392,14 @@ class DocumentLevelMonitorRunner : MonitorRunner() {
                RestStatus.INTERNAL_SERVER_ERROR,
                e
            )
+            if (monitor.dataSources.queryIndex.contains("optimized") &&


plz dont write this twice
move this to finally block

eirsep · 2024-09-25T06:23:24Z

alerting/src/main/kotlin/org/opensearch/alerting/service/DeleteMonitorService.kt

@@ -95,57 +95,69 @@ object DeleteMonitorService :

    private suspend fun deleteDocLevelMonitorQueriesAndIndices(monitor: Monitor) {
        try {
-            val metadata = MonitorMetadataService.getMetadata(monitor)
-            metadata?.sourceToQueryIndexMapping?.forEach { (_, queryIndex) ->
+            if (monitor.owner == "alerting") {


plz delete based on a boolean flag that defaults to false.
This is an anti pattern and makes code unmaintainable

fixed it based on boolean flag passed from common-utils.

eirsep · 2024-09-25T06:40:20Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexMonitorAction.kt

@@ -537,7 +537,8 @@ class TransportIndexMonitorAction @Inject constructor(
                    if (
                        request.monitor.isMonitorOfStandardType() &&
                        Monitor.MonitorType.valueOf(request.monitor.monitorType.uppercase(Locale.ROOT)) ==
-                        Monitor.MonitorType.DOC_LEVEL_MONITOR
+                        Monitor.MonitorType.DOC_LEVEL_MONITOR &&
+                        request.monitor.owner == "alerting"
                    ) {
                        indexDocLevelMonitorQueries(request.monitor, indexResponse.id, metadata, request.refreshPolicy)
                    }


https://github.com/opensearch-project/alerting/pull/1664/files#diff-58c6e3aa339ebbcc7ee2452426f17164dc91d435c1f24ee55fdc4f06ec1a371eR546

here source to query mapping is updated but in case of new change query index doesnt even get created and wrong value would come up right?
this needs to be handled too

the source to query mapping is dynamically updated during the first monitor run as shown in this logic. https://github.com/opensearch-project/alerting/blob/main/alerting/src/main/kotlin/org/opensearch/alerting/util/DocLevelMonitorQueries.kt#L481

is this method being called for the monitors with dedicated queryindex also?
https://github.com/opensearch-project/alerting/blob/main/alerting/src/main/kotlin/org/opensearch/alerting/util/DocLevelMonitorQueries.kt#L473C25-L473C49

this code snippet will always throw exception right?

targetQueryIndex = getWriteIndexNameForAlias(monitor.dataSources.queryIndex) if (targetQueryIndex == null) { val message = "Failed to get write index for queryIndex alias:${monitor.dataSources.queryIndex}" log.error(message) throw AlertingException.wrap( OpenSearchStatusException(message, RestStatus.INTERNAL_SERVER_ERROR) ) }

because the queryIndex is not an alias anymore?

eirsep · 2024-09-25T06:41:12Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexMonitorAction.kt

+                                .execute(it)
+                        }
+                    }
+                    if (currentMonitor.owner == "alerting") {


refactor to use flag

eirsep · 2024-09-25T06:43:04Z

alerting/src/main/kotlin/org/opensearch/alerting/util/DocLevelMonitorQueries.kt

@@ -181,6 +182,16 @@ class DocLevelMonitorQueries(private val client: Client, private val clusterServ
        }
    }

+    suspend fun deleteDocLevelQueryIndex(dataSources: DataSources): Boolean {
+        val ack: AcknowledgedResponse = client.suspendUntil {


can we do exists check before deleting

eirsep

plz remove request.monitor.owner == "alerting" and replace with a flag from monitor creation that defaults to false.
Boolean flag should determine if query index should be deleted in every run

Signed-off-by: Subhobrata Dey <[email protected]>

sbcd90 · 2024-09-25T10:13:32Z

hi @eirsep , removed equest.monitor.owner == "alerting" and replaced with a flag from monitor creation that defaults to false.

eirsep

scheduled jobs json mapping needs to be updated

Signed-off-by: Subhobrata Dey <[email protected]>

sbcd90 · 2024-09-25T21:09:12Z

scheduled jobs json mapping needs to be updated

fixed this.

Signed-off-by: Subhobrata Dey <[email protected]>

eirsep · 2024-09-25T22:17:31Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexMonitorAction.kt

@@ -537,7 +537,8 @@ class TransportIndexMonitorAction @Inject constructor(
                    if (
                        request.monitor.isMonitorOfStandardType() &&
                        Monitor.MonitorType.valueOf(request.monitor.monitorType.uppercase(Locale.ROOT)) ==
-                        Monitor.MonitorType.DOC_LEVEL_MONITOR
+                        Monitor.MonitorType.DOC_LEVEL_MONITOR &&
+                        request.monitor.owner == "alerting"
                    ) {
                        indexDocLevelMonitorQueries(request.monitor, indexResponse.id, metadata, request.refreshPolicy)
                    }


is this method being called for the monitors with dedicated queryindex also?
https://github.com/opensearch-project/alerting/blob/main/alerting/src/main/kotlin/org/opensearch/alerting/util/DocLevelMonitorQueries.kt#L473C25-L473C49

this code snippet will always throw exception right?

targetQueryIndex = getWriteIndexNameForAlias(monitor.dataSources.queryIndex) if (targetQueryIndex == null) { val message = "Failed to get write index for queryIndex alias:${monitor.dataSources.queryIndex}" log.error(message) throw AlertingException.wrap( OpenSearchStatusException(message, RestStatus.INTERNAL_SERVER_ERROR) ) }

because the queryIndex is not an alias anymore?

eirsep · 2024-09-25T22:20:58Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexMonitorAction.kt

@@ -711,7 +711,7 @@ class TransportIndexMonitorAction @Inject constructor(
                                .execute(it)
                        }
                    }
-                    if (currentMonitor.owner == "alerting") {
+                    if (currentMonitor.deleteQueryIndexInEveryRun == false) {


alerting/alerting/src/main/kotlin/org/opensearch/alerting/util/DocLevelMonitorQueries.kt

Line 459 in b280761

log.debug(bulkItemResponse.failureMessage)

plz change it to error log

eirsep · 2024-09-25T22:25:15Z

alerting/src/test/kotlin/org/opensearch/alerting/MonitorDataSourcesIT.kt

-            triggers = listOf(trigger)
+            triggers = listOf(trigger),
+            dataSources = DataSources(),
+            owner = "alerting"


plz add 2 new tests-

scenario where delete query index flag is false and monitor execution fails due to mapping conflict on alias rollover

scenario where delete query index flag is true and monitor execution succeeds

in first test plz also update monitor with flag false to true and do rollover again and this time assert monitor execution succeeeds

sbcd90 · 2024-09-25T23:10:16Z

#1664 (comment)
query index is same name as query index alias.

eirsep

follow up PR with tests menttioned and error message

…ors (#1664) Signed-off-by: Subhobrata Dey <[email protected]> (cherry picked from commit abdeca9) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…ors (#1664) (#1667) Signed-off-by: Subhobrata Dey <[email protected]>

…ors (#1664) Signed-off-by: Subhobrata Dey <[email protected]> (cherry picked from commit abdeca9) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…ors (opensearch-project#1664) (opensearch-project#1667) Signed-off-by: Subhobrata Dey <[email protected]>

separate doc-level monitor query indices for externally defined monitors

452a51c

Signed-off-by: Subhobrata Dey <[email protected]>

sbcd90 requested review from lezzago, AWSHurneyt, eirsep, getsaurabh02, praveensameneni, bowenlan-amzn, rishabhmaurya, engechas, riysaxen-amzn, jowg-amazon, amsiglan and goyamegh as code owners September 25, 2024 03:17

eirsep reviewed Sep 25, 2024

View reviewed changes

eirsep requested changes Sep 25, 2024

View reviewed changes

separate doc-level monitor query indices for externally defined monitors

b280761

Signed-off-by: Subhobrata Dey <[email protected]>

eirsep requested changes Sep 25, 2024

View reviewed changes

update source to index mappings for detector upgrade scenarios

a15d5a1

Signed-off-by: Subhobrata Dey <[email protected]>

fix ktlint

90620a0

Signed-off-by: Subhobrata Dey <[email protected]>

eirsep reviewed Sep 25, 2024

View reviewed changes

eirsep requested changes Sep 25, 2024

View reviewed changes

eirsep approved these changes Sep 25, 2024

View reviewed changes

jowg-amazon approved these changes Sep 25, 2024

View reviewed changes

sbcd90 merged commit abdeca9 into opensearch-project:main Sep 25, 2024
18 checks passed

sbcd90 added backport 2.x backport 2.15 backport 2.17 labels Sep 26, 2024

opensearch-trigger-bot bot mentioned this pull request Sep 26, 2024

[Backport 2.15] separate doc-level monitor query indices for externally defined monitors #1665

Merged

opensearch-trigger-bot bot mentioned this pull request Sep 26, 2024

[Backport 2.17] separate doc-level monitor query indices for externally defined monitors #1666

Closed

opensearch-trigger-bot bot mentioned this pull request Sep 26, 2024

[Backport 2.x] separate doc-level monitor query indices for externally defined monitors #1667

Merged

sbcd90 pushed a commit that referenced this pull request Sep 26, 2024

separate doc-level monitor query indices for externally defined monit…

dcf8178

…ors (#1664) (#1667) Signed-off-by: Subhobrata Dey <[email protected]>

sbcd90 mentioned this pull request Sep 26, 2024

move deletion of query index before its creation #1668

Merged

5 tasks

sbcd90 added the backport 2.16 label Sep 26, 2024

opensearch-trigger-bot bot mentioned this pull request Sep 26, 2024

[Backport 2.16] separate doc-level monitor query indices for externally defined monitors #1679

Open

sbcd90 pushed a commit to sbcd90/alerting that referenced this pull request Oct 16, 2024

separate doc-level monitor query indices for externally defined monit…

67898f1

…ors (opensearch-project#1664) (opensearch-project#1667) Signed-off-by: Subhobrata Dey <[email protected]>

sbcd90 mentioned this pull request Oct 16, 2024

separate doc-level monitor query indices for externally defined monitors #1699

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

separate doc-level monitor query indices for externally defined monitors #1664

separate doc-level monitor query indices for externally defined monitors #1664

sbcd90 commented Sep 25, 2024 •

edited by eirsep

Loading

eirsep Sep 25, 2024

sbcd90 Sep 25, 2024

eirsep Sep 25, 2024

sbcd90 Sep 25, 2024

eirsep Sep 25, 2024

sbcd90 Sep 25, 2024

eirsep Sep 25, 2024

sbcd90 Sep 25, 2024

eirsep Sep 25, 2024

sbcd90 Sep 25, 2024

eirsep Sep 25, 2024 •

edited

Loading

eirsep Sep 25, 2024

sbcd90 Sep 25, 2024

eirsep Sep 25, 2024

sbcd90 Sep 25, 2024

eirsep left a comment

sbcd90 commented Sep 25, 2024

eirsep left a comment

sbcd90 commented Sep 25, 2024

eirsep Sep 25, 2024 •

edited

Loading

eirsep Sep 25, 2024

eirsep Sep 25, 2024

eirsep Sep 25, 2024

sbcd90 commented Sep 25, 2024

eirsep left a comment

separate doc-level monitor query indices for externally defined monitors #1664

separate doc-level monitor query indices for externally defined monitors #1664

Conversation

sbcd90 commented Sep 25, 2024 • edited by eirsep Loading

Description

Related Issues

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eirsep Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eirsep left a comment

Choose a reason for hiding this comment

sbcd90 commented Sep 25, 2024

eirsep left a comment

Choose a reason for hiding this comment

sbcd90 commented Sep 25, 2024

eirsep Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbcd90 commented Sep 25, 2024

eirsep left a comment

Choose a reason for hiding this comment

sbcd90 commented Sep 25, 2024 •

edited by eirsep

Loading

eirsep Sep 25, 2024 •

edited

Loading

eirsep Sep 25, 2024 •

edited

Loading